ci: drop node.dev sanitization step (after genlayer-node NOD-973)#430
ci: drop node.dev sanitization step (after genlayer-node NOD-973)#430dohernandez wants to merge 2 commits into
Conversation
genlayer-node NOD-973 removes the `node.dev` section (its only field, `disableSubscription`, a test-only toggle) entirely from config.yaml.example. Once that lands, `del(.node.dev)` deletes a non-existent path — dead config. Remove the del step and the now-stale trailing yq pipe; the RPC-URL/provider placeholder substitutions are unrelated and stay. Sync the workflows README accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for genlayer-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Config Sanitization Behavior Change
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/sanitize-config.sh:
- Around line 23-27: The sanitize-config script currently assumes `.node.dev`
has already been removed, but `sanitize-config.sh` can still run while that
field exists and then propagate an invalid config. Update the script to
explicitly check the input config for `.node.dev` before the `yq` replacements
run and abort with a clear failure if it is still present. Use the existing
`yq`-based flow in `sanitize-config.sh` and keep the guard close to the current
placeholder replacement logic so the sequencing requirement is enforced in code
rather than by merge order.
In @.github/workflows/README.md:
- Around line 184-189: The README summary for the sanitization scripts is
outdated because sanitize-config.sh now also rewrites .rollup.provider, not just
URLs. Update the “Scripts Used” one-line description to mention provider
sanitization as well, and make sure the wording stays consistent with the Config
Sanitization section so the summary matches the actual behavior of
.github/scripts/sanitize-config.sh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7252106f-4aba-4fa3-b927-b737f812e488
📒 Files selected for processing (2)
.github/scripts/sanitize-config.sh.github/workflows/README.md
| # Replace RPC URLs / provider with TODO placeholders | ||
| yq -i ' | ||
| .rollup.genlayerchainrpcurl = "TODO: Set your GenLayer Chain ZKSync HTTP RPC URL here" | | ||
| .rollup.genlayerchainwebsocketurl = "TODO: Set your GenLayer Chain ZKSync WebSocket RPC URL here" | | ||
| .rollup.provider = "TODO: Set your GenLayer Chain ZKSync provider" | | ||
| del(.node.dev) | ||
| .rollup.provider = "TODO: Set your GenLayer Chain ZKSync provider" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail fast if upstream still ships node.dev.
.github/actions/sync-files/action.yml:40-60 always syncs this script’s output downstream, so once del(.node.dev) is removed, merging this early will silently publish a config that still contains node.dev. Please enforce the sequencing requirement in code by aborting when .node.dev is still present instead of relying on merge timing alone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/sanitize-config.sh around lines 23 - 27, The sanitize-config
script currently assumes `.node.dev` has already been removed, but
`sanitize-config.sh` can still run while that field exists and then propagate an
invalid config. Update the script to explicitly check the input config for
`.node.dev` before the `yq` replacements run and abort with a clear failure if
it is still present. Use the existing `yq`-based flow in `sanitize-config.sh`
and keep the guard close to the current placeholder replacement logic so the
sequencing requirement is enforced in code rather than by merge order.
There was a problem hiding this comment.
This is fine, it is set in the PR description
There was a problem hiding this comment.
@dohernandez Understood, thanks for clarifying. Since the sequencing requirement is already captured in the PR description, I won’t push further on adding the in-script guard in this PR.
Address CodeRabbit: the "Scripts Used" entry for sanitize-config.sh still said "replaces URLs" while the detailed Config Sanitization section lists provider too. Align the summary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
v0.6Merging this early would leak the un-sanitized
node.devsection into the published docs config. Until NOD-973 merges, upstreamconfig.yaml.examplestill shipsnode.dev, so ourdel(.node.dev)is still doing real sanitization. This PR is only safe to merge oncenode.devis gone from the synced source.What
genlayer-node NOD-973 removes the
node.devsection entirely fromconfig.yaml.example. Its only field wasdisableSubscription(a test-only toggle for zksync-anvil). Once removed upstream, ourdel(.node.dev)in the config sanitizer deletes a non-existent path — dead config.Changes
.github/scripts/sanitize-config.sh— drop thedel(.node.dev)step and the now-trailing yq pipe (|) it dangled off. RPC-URL + provider placeholder substitutions are unrelated and unchanged..github/workflows/README.md— remove the two stale references to "removes dev section" / "Dev Section Removal".Coordination
Confirmed sequencing with the genlayer-node session working NOD-973 (handoff, 2026-06-29). They'll signal when it lands on
v0.6; this PR should merge after that.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation